Tempo control refactor with arpeggiator, synth, and UI/docs #39

brsbl · 2025-12-11T09:56:52Z

Summary

Refactor introducing tempo-controlled arpeggiator integration, polyphonic synth engine, and expanded UI structure.
Add real-time audio visualizers that respond to both drum and synth output.
Restructure UI into explicit Drums and Synth sections with per-track labels (HI-HAT, SNARE, KICK) and per-track knobs for volume, reverb, and filter.
Introduce 2-octave keyboard input with QWERTY support and latch behavior for held notes.
Expand test coverage to verify tempo-synced arpeggiation, waveform changes, keyboard interactions, and UI alignment.
Groundwork for Tailwind CSS v4 migration and new design system/docs (PRD, ROADMAP, DESIGN SYSTEM, STYLE GUIDE).
Maintain default development server port at 3000.
Add scaffolding and tooling updates to support architecture improvements and accessibility enhancements.

What changed

Arpeggiator and tempo control now serve as the tempo-synced core of playback, with direction modes (up, down, up-down) and note-rate options (1/4, 1/8, 1/16).
Polyphonic synth engine added with waveform selection (sawtooth, square, sine, triangle) and integration with the keyboard input.
2-octave keyboard input introduced with click-to-toggle for held notes and QWERTY mapping support.
Real-time audio visualizers added and wired to respond to both drum and synth sounds.
UI restructuring to clearly separate drums and synth sections, including per-track labels and per-track control knobs.
UI alignment improvements for drum pads, arp pads, and keyboard keys; improved right-column layout for knobs and tempo controls.
Architectural and design system documentation added: PRD, ROADMAP, DESIGN SYSTEM, and STYLE GUIDE.
Tailwind v4 migration groundwork and utility helpers introduced (structure prepared for incremental migration).
Git hooks, CI/test scaffolding, and LFS considerations updated to support larger, design-system-driven changes.

Files touched (high level)

src/components/ArpTrack.jsx, src/components/AudioVisualizer.jsx, src/components/DrumMachine.jsx, src/components/Key.jsx, src/components/Keyboard.jsx, src/hooks/useArpeggiator.js, src/hooks/useAudioEngine.js, src/hooks/useSynthEngine.js, src/hooks/useSequencer.js, src/components/Sequencer.jsx, src/components/Track.jsx, src/components/TransportControls.jsx, src/components/ui/Fader.jsx, src/index.css
PRD.md, docs/RECOMMENDATIONS.md, docs/ROADMAP.md, docs/design/DESIGN_SYSTEM.md, docs/design/STYLE_GUIDE.md, docs/design/color_system.png, docs/design/text_styles.png, package.json, pnpm-lock.yaml, 53+ files updated/added
.claude/agents/* (documentation of reviewers and checklists for ongoing PR reviews)

Test plan

🤖 Generated with Claude Code

📎 Task: https://www.terragonlabs.com/task/7110b41e-3846-4458-aa5f-2403afeab236

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Make BPM stateful in useSequencer hook with real-time tempo changes - Add editable number input for BPM (60-200 range) - Persist BPM in URL state (backwards compatible with old URLs) - Set up Vitest testing infrastructure - Add tests for TransportControls, Pad, instruments config, and URL state - Include tempo change tests for input validation and blur/Enter behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Add 16-step arpeggiator with up/down/up-down modes and rate control - Add polyphonic synth engine with waveform selection (saw/square/sine/triangle) - Add 2-octave keyboard with toggle mode for held notes - Add audio visualizers that respond to playing sounds - Restructure UI layout with drums section and synth section - Add track labels (HI-HAT, SNARE, KICK) - Add per-track volume/reverb/filter knobs - Improve container alignment with fit-content sizing - Set default dev server port to 3000 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

vercel · 2025-12-11T09:56:56Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
loop-machine	Ready	Preview	Comment	Dec 11, 2025 10:27am

brsbl

Code Review Summary

I've reviewed the PR and found one blocking issue that will cause CI failures. The overall implementation of the arpeggiator, synth engine, and UI improvements is well-structured.

Overall Correctness Verdict: INCORRECT

The patch contains a bug in the CI configuration that will cause the workflow to fail on every run. The workflow references npm scripts that do not exist in package.json.

brsbl

Code Review Summary

I've reviewed the PR and found one blocking issue that will cause CI failures. The overall implementation of the arpeggiator, synth engine, and UI improvements is well-structured.

Overall Correctness Verdict: INCORRECT

The patch contains a bug in the CI configuration that will cause the workflow to fail on every run. The workflow references npm scripts that do not exist in package.json.

brsbl · 2025-12-11T09:58:45Z

.github/workflows/ci.yml

+      run: npm run lint
+
+    - name: Run tests
+      run: npm run test:coverage


[P0] CI workflow references non-existent npm scripts

The CI workflow calls npm run format:check, npm run lint, and npm run test:coverage, but none of these scripts exist in package.json. The package.json only defines test and test:watch scripts. This will cause every CI run to fail immediately with an error like npm ERR! Missing script: "format:check".

Either add the missing scripts to package.json, or update the CI workflow to use existing scripts (e.g., npm run test instead of npm run test:coverage).

Resolve merge conflicts: - Keep vitest for testing (over jest) - Merge UI dependencies from main - Add tw-animate-css and theme variables - Add path alias for @ imports - Combine BPM persistence with filter settings in urlState 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

brsbl · 2025-12-11T10:02:51Z

src/hooks/useSynthEngine.js

+      gainNode.connect(masterGainRef.current)
+
+      // Start oscillator
+      oscillator.start(now)


[P0] Unsynchronized playback

The synth engine triggers notes immediately using ctx.currentTime. This ignores the precise scheduling time (nextNoteTimeRef) calculated by the sequencer lookahead. This will cause the synth to play out of sync with the drums (approximately scheduleAheadTime early) and suffer from main-thread timing jitter.

playNote should accept a time parameter and use it: oscillator.start(time).

brsbl · 2025-12-11T10:02:51Z

src/hooks/useArpeggiator.js

+    }
+
+    // Play new note
+    synth.playNote('arp-' + noteName, noteData.freq)


[P0] Missing timing parameter

The noteTime parameter (calculated for precise scheduling) is received here but not passed to synth.playNote. This prevents the synth from playing in sync with the sequencer grid.

Suggested change

synth.playNote('arp-' + noteName, noteData.freq)

synth.playNote('arp-' + noteName, noteData.freq, noteTime)

brsbl · 2025-12-11T10:02:51Z

src/hooks/useAudioEngine.js

    const nodes = effectNodesRef.current[instrumentId]
    if (!nodes || !audioContextRef.current) return

+    const sliderValue = parseInt(value, 10)


[P0] Broken effects control

parseInt will truncate the 0-1 float values from the UI sliders to either 0 or 1, effectively breaking the reverb and delay controls (which will mostly stay at 0). Additionally, the subsequent division by 10 implies the code expects 0-10 or 0-100, but the sliders provide 0-1.

Suggested change

const sliderValue = parseInt(value, 10)

const sliderValue = parseFloat(value)

brsbl · 2025-12-11T10:02:51Z

src/hooks/useAudioEngine.js

-        nodes.lowpassFilter.frequency.setValueAtTime(cutoff, currentTime)
-        break
-      }
+    if (effectType === 'reverb') {


[P1] Missing Filter implementation

The lowpassFilter node and its control logic have been removed from the audio graph, but the UI still exposes a Filter knob that calls this function with effectType === 'filter'. Currently, the filter knob does nothing.

brsbl · 2025-12-11T10:02:51Z

src/components/DrumMachine.jsx

          onReset={sequencer.resetPattern}
+          arpeggiator={arpeggiator}
+          synth={synth}
+          analyser={audioEngine.analyser}


[P2] Visualizer missing synth

The visualizer is connected only to the audioEngine (drums) analyser. The synth engine uses a separate AudioContext and signal chain, so synth notes will not appear in the visualizer.

- Convert useAudioEngine.test.js from Jest to Vitest imports - Update urlState.test.js to match new encoding format (volume/reverb/filter) - Fix Button component default export (remove shadcn conflict) - Add ResizeObserver global declaration in AudioVisualizer 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

The ci.yml workflow already handles testing with pnpm. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

brsbl · 2025-12-11T10:18:12Z

@terragon-labs please use subagents to review all PR feedback, create a plan to address each one, then have additional subagents review each fix

…alizer - Add optional time parameter to useSynthEngine.playNote() for precise Web Audio scheduling instead of immediate ctx.currentTime - Pass noteTime from arpeggiator to synth.playNote() for grid sync - Fix effects control: change parseInt to parseFloat for 0-1 slider values and remove incorrect /10 division in reverb/delay calculations - Restore lowpassFilter implementation with exponential frequency mapping (200Hz-20kHz) and proper signal chain routing - Share AudioContext between drum and synth engines so visualizer displays both audio sources 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

brsbl · 2025-12-11T10:26:38Z

PR Review Fixes Applied ✅

I've reviewed all the PR feedback using subagents and implemented fixes for each issue:

Issues Fixed

Issue	Priority	Status	Fix
CI workflow scripts	P0	✅ Already resolved	Scripts exist in package.json
Unsynchronized synth playback	P0	✅ Fixed	Added optional `time` parameter to `playNote()`
Missing arpeggiator timing	P0	✅ Fixed	Pass `noteTime` to `synth.playNote()`
Broken effects control	P0	✅ Fixed	`parseInt` → `parseFloat`, removed `/10` division
Missing Filter implementation	P1	✅ Fixed	Added lowpassFilter node with exponential freq mapping
Visualizer missing synth	P2	✅ Fixed	Shared AudioContext between drum/synth engines

Changes Made

src/hooks/useSynthEngine.js

Added optional time parameter to playNote(noteId, frequency, time)
Uses time ?? ctx.currentTime for Web Audio scheduling
Accepts shared AudioContext and masterGain for visualizer integration

src/hooks/useArpeggiator.js

Pass noteTime to synth.playNote() for grid sync

src/hooks/useAudioEngine.js

Fixed parseInt → parseFloat for 0-1 slider values
Removed incorrect /10 division in effect calculations
Added lowpassFilter BiquadFilter node (200Hz-20kHz exponential mapping)
Exposed audioContext and masterGain for sharing

src/components/DrumMachine.jsx

Pass shared context to synth engine

Verification

All 80 tests passing ✅
Each fix reviewed by senior-code-reviewer subagent
Commit: 1d74ff0

…encies Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

brsbl

Code Review Summary

I've reviewed this PR which introduces a tempo-controlled arpeggiator, polyphonic synth engine, expanded UI structure, and audio visualizers.

Overall Assessment

The code is well-structured with good separation of concerns:

React hooks are properly designed with refs for non-reactive state and useCallback for memoization
Audio engine properly handles Web Audio API lifecycle (context creation, cleanup on unmount)
The synth engine correctly handles shared vs standalone AudioContext modes
URL state encoding/decoding is robust with backward compatibility
Tests provide reasonable coverage for the core functionality

Findings

No blocking bugs identified. The implementation appears solid with proper cleanup, error handling, and consideration for edge cases like:

Multiple audio context initialization prevention via initCountRef
Proper event listener cleanup in hooks
Suspended AudioContext handling for browser autoplay policies
Backward compatibility for URL state format changes

The code follows React best practices and the audio scheduling is implemented correctly using the standard web audio scheduler pattern.

Verdict: ✅ Correct

The patch does not introduce bugs that would break existing functionality or tests.

brsbl

Reviewing the changes. There are a few issues to address regarding the arpeggiator logic and potential performance risks.

brsbl · 2025-12-11T19:38:01Z

[P1] Arpeggiator notes sustain indefinitely over empty steps

In useArpeggiator.js, scheduleArpNote returns early if !pattern[currentStep] or if the step doesn't match the rate. It effectively skips scheduling logic for that step.

However, synth.playNote starts an oscillator with a sustain level (0.7) that continues until stopNote is explicitly called. Currently, stopNote is only called immediately before playing a new note.

This means if the pattern has gaps (e.g., ON - OFF - ON), the first note will sustain through the OFF step, acting like a tie/slide. It is impossible to program rests or staccato notes.

Suggestion:
Handle the "gate closed" case to explicitly stop the previous note.

    // Only play on matching steps
    if (currentStep % skipSteps !== 0) {
      return
    }

    // Check if gate is open for this step
    if (!pattern[currentStep]) {
      // Gate is closed. Stop previous note if it's still playing.
      if (lastPlayedNoteRef.current) {
        synth.stopNote('arp-' + lastPlayedNoteRef.current)
        lastPlayedNoteRef.current = null
      }
      return
    }

brsbl · 2025-12-11T19:38:13Z

[P2] Potential AudioContext re-initialization on render

In useAudioEngine.js, the useEffect dependency array includes instruments.

  useEffect(() => {
    // ... initAudio ...
  }, [instruments])

If the instruments prop passed to DrumMachine is not referentially stable (e.g., defined inline in the parent component or created on every render), this effect will re-run on every render, causing the AudioContext to close and re-initialize repeatedly. This would cause severe audio glitches.

Suggestion:
Ensure instruments is stable, or use JSON.stringify(instruments) or deep comparison in the dependency array if stability cannot be guaranteed by the caller. Alternatively, confirm that instruments is a constant in App.jsx.

brsbl and others added 4 commits December 10, 2025 17:50

Add PRD.md to track project roadmap

bb6cf7a

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add husky git hooks configuration

d13e1e5

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

brsbl commented Dec 11, 2025

View reviewed changes

vercel bot deployed to Preview December 11, 2025 10:02 View deployment

brsbl commented Dec 11, 2025

View reviewed changes

vercel bot deployed to Preview December 11, 2025 10:13 View deployment

Remove duplicate test.yml workflow (uses npm instead of pnpm)

85a616d

The ci.yml workflow already handles testing with pnpm. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

vercel bot deployed to Preview December 11, 2025 10:16 View deployment

vercel bot deployed to Preview December 11, 2025 10:26 View deployment

chore(dependencies): add initial package-lock.json for project depend…

2856861

…encies Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

vercel bot deployed to Preview December 11, 2025 10:27 View deployment

brsbl changed the title ~~Add arpeggiator, synth engine, and UI improvements~~ Tempo control refactor with arpeggiator, synth, and UI/docs Dec 11, 2025

brsbl commented Dec 11, 2025

View reviewed changes

brsbl merged commit ca1ecac into main Dec 11, 2025
5 checks passed

	synth.playNote('arp-' + noteName, noteData.freq)
	synth.playNote('arp-' + noteName, noteData.freq, noteTime)

	const sliderValue = parseInt(value, 10)
	const sliderValue = parseFloat(value)

Tempo control refactor with arpeggiator, synth, and UI/docs #39

Tempo control refactor with arpeggiator, synth, and UI/docs #39

Uh oh!

Conversation

brsbl commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Files touched (high level)

Test plan

Uh oh!

vercel bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brsbl left a comment

Choose a reason for hiding this comment

Code Review Summary

Uh oh!

brsbl left a comment

Choose a reason for hiding this comment

Code Review Summary

Uh oh!

brsbl Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

brsbl Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

brsbl Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

brsbl Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

brsbl Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

brsbl Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

brsbl commented Dec 11, 2025

Uh oh!

brsbl commented Dec 11, 2025

PR Review Fixes Applied ✅

Issues Fixed

Changes Made

Verification

Uh oh!

brsbl left a comment

Choose a reason for hiding this comment

Code Review Summary

Overall Assessment

Findings

Uh oh!

brsbl left a comment

Choose a reason for hiding this comment

Uh oh!

brsbl commented Dec 11, 2025

Uh oh!

brsbl commented Dec 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

brsbl commented Dec 11, 2025 •

edited

Loading

vercel bot commented Dec 11, 2025 •

edited

Loading